Fix running of Q28 by not removing important backslashes from regex #252
Conversation
|
|
Q28 contains backslashes, specifically the backslash in `\1` is very important for the meaning of the query. However, both backslashes in this query are accidentally removed in almost all benchmark scripts because they used `read` instead of `read -r`. This adds the `-r` flag to all the scripts that didn't have it yet, to make sure that `read` interprets it as a raw `\` character instead of an escape sequence. With this change the REGEXP_REPLACE call doesn't always return the literal string "1", but actually (as intended by the query) the referrer domain. This change generally makes the query significantly slower to execute, because now there are many more groups. I'm not sure how this should be listed in the results UI. It might make sense to ignore the query or something. This problem was found independently in ClickHouse#47 and ClickHouse#72, but fixed in a different way. I'm not sure if the fix that was made for starrocks in ClickHouse#72 is actually correct though, because it seems to fix the issue twice: It replaces `\1` with `\\1`, but it also added the `-r` flag to `read`. Apart from those issues there were already a few benchmarks that were using the `-r` flag: ``` ❯ rg 'while read -r' elasticsearch/run.sh 5:cat 'queries.sql' | while read -r QUERY; do starrocks/run.sh 8:cat queries.sql | while read -r query; do doris/run.sh 8:while read -r query; do crunchy-bridge-for-analytics/run.sh 22: while read -r query; do ```
|
@JelteF That is a great find. Regarding Doris and Starrocks: Let me ping the contributors who last touched Q28 for these two databases. It is indeed weird that they seemingly fixed the issue redundantly. Now, all Regarding re-running benchmarks: that is too much work (at least for me) and as measurements will be updated over time, the effect of the bug on Q28 should diminish. Also, just one out of 43 queries is affected so aggregated results should not be too much off. Of course, if someone likes to re-run benchmarks, please go ahead. |
|
MySQL supports C-style escaping, and the databases implementing the MySQL query dialect, such as MariaDB, Infobright, SingleStore, Doris/Starrocks, are expected to parse this escaping. However, it can be modified by sql_mode: https://dev.mysql.com/doc/refman/8.4/en/sql-mode.html#sqlmode_no_backslash_escapes ClickHouse supports C-style escaping, but does it in an intelligent way, so when there is |
Q28 contains backslashes, specifically the backslash in
\1is very important for the meaning of the query. However, both backslashes in this query are accidentally removed in almost all benchmark scripts because they usedreadinstead ofread -r.This adds the
-rflag to all the scripts that didn't have it yet, to make sure thatreadinterprets it as a raw\character instead of an escape sequence.With this change the
REGEXP_REPLACEcall doesn't always return the literal string "1", but actually (as intended by the query) the referrer domain. This change generally makes the query significantly slower to execute, because now there are many more groups. I'm not sure how this should be listed in the results UI. It might make sense to ignore the query or something until all/most benchmarks have been rerun.This problem was found independently in #47 and #72, but fixed in a different way. I'm not sure if the fix that was made for starrocks in #72 is actually correct though, because it seems to fix the issue twice: It replaces
\1with\\1, but it also added the-rflag toread. The same is the case for thedorisbenchmark.Apart from those PRs there were already a few benchmarks that were using the
-rflag: